-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Implement an upgrade plugin to update passwords hashing algorithm - EXO-65151 - Meeds-io/MIPs#69 #161
Conversation
def7e0d
to
1ee0c79
Compare
data-upgrade-users/src/main/java/org/exoplatform/migration/UserPasswordHashMigration.java
Outdated
Show resolved
Hide resolved
|
||
@Override | ||
public void afterUpgrade() { | ||
settingService.set(Context.GLOBAL.id(USER_PASSWORD_HASH_MIGRATION), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be set only if success.
In your case, if there are 10k users to update, and if I stop the server before the end, it will set it as finished. but it is not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't understand, if you stop the server this will not be called, thus i added it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that afterUpgrade is called, even the upgrade is not finished or failed.
Other question : imagine that during the execution, there is a problem for some users (not all).
Then the execution finished.
Does the migration is considered as finished or not ?
Does it will restart at next startup ?
In addition, you configure upgradePlugin as executed once, so why do we need to store a setting to know if the execution is finished ?
In fact, in other upgrade plugin, there are 2 option
- UP finish with an exeception : so it is considered as not executed and will relaunched at next startup
- UP finish without exception : so it is considered as executed, and will not relaunched at next startup
With this assumption, do we really need a setting to mark it as finished ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that afterUpgrade is called, even the upgrade is not finished or failed.
Other question : imagine that during the execution, there is a problem for some users (not all).
Then the execution finished. Does the migration is considered as finished or not ? Does it will restart at next startup ?
In addition, you configure upgradePlugin as executed once, so why do we need to store a setting to know if the execution is finished ?
In fact, in other upgrade plugin, there are 2 option
- UP finish with an exeception : so it is considered as not executed and will relaunched at next startup
- UP finish without exception : so it is considered as executed, and will not relaunched at next startup
With this assumption, do we really need a setting to mark it as finished ?
@rdenarie I hope this replies to your comments
https://github.com/exoplatform/data-upgrade/pull/161/files#diff-1a9349cb98114358fe6ed02277b76ad326d706337cec3efe4e732b4f6ed39b41R119-R128
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no sure. Need to speak about that. Can you ping me ?
data-upgrade-users/src/test/java/org/exoplatform/migration/UserPasswordHashMigrationTest.java
Outdated
Show resolved
Hide resolved
proceedToUpgrade = userPasswordHashMigration.shouldProceedToUpgrade(null, null, null); | ||
assertFalse(proceedToUpgrade); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add a test with an error during process upgrade, to check that the upgrade correctly restart ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
already exists by not calling the afterUpgrade, the shouldProceedToUpgrade is true, so when inttrepring server start or stop the server during the UP execution , the afterUpgrade will not be called, so the next restart the UP will be launched again .
https://github.com/exoplatform/data-upgrade/pull/161/files#diff-1a9349cb98114358fe6ed02277b76ad326d706337cec3efe4e732b4f6ed39b41R100
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, my test would be
boolean upgrade = processUpgrade
//with mock find a way to fail it
assertFalse(upgrade)
//restart processUpgrade
assert(process upgrade correctly restart and finish)
proceedToUpgrade = userPasswordHashMigration.shouldProceedToUpgrade(null, null, null); | ||
assertFalse(proceedToUpgrade); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, my test would be
boolean upgrade = processUpgrade
//with mock find a way to fail it
assertFalse(upgrade)
//restart processUpgrade
assert(process upgrade correctly restart and finish)
|
||
@Override | ||
public void afterUpgrade() { | ||
settingService.set(Context.GLOBAL.id(USER_PASSWORD_HASH_MIGRATION), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that afterUpgrade is called, even the upgrade is not finished or failed.
Other question : imagine that during the execution, there is a problem for some users (not all).
Then the execution finished.
Does the migration is considered as finished or not ?
Does it will restart at next startup ?
In addition, you configure upgradePlugin as executed once, so why do we need to store a setting to know if the execution is finished ?
In fact, in other upgrade plugin, there are 2 option
- UP finish with an exeception : so it is considered as not executed and will relaunched at next startup
- UP finish without exception : so it is considered as executed, and will not relaunched at next startup
With this assumption, do we really need a setting to mark it as finished ?
4065a47
to
d909a19
Compare
@rdenarie PR updated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also fix sonar feedbacks.
|
||
private String getDefaultCredentialEncoderName() { | ||
Map<String, String> props = new HashMap<>(); | ||
picketLinkIDMService.getConfigMD().getRealms().get(0).getOptions().forEach((k, v) -> props.put(k, v.get(0))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain this line ?
There is twice get(0), are we ok with the fact that there is only one realm ?
and that the option we search is really in first position ?
In addition, instead of creating a map<String, String>, could we use a filter/map operation to directly have the correct value to return ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is the only way to do that, and it is one realm so it is in the first position,
if you have another way could you suggest it ?
and i don't see how to use filter/map here and what difference will make.
Do you know that options is a map and not a list or array ? beside that the value of the map is another list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a solution to get default credential with proper way and is to declare a public function: ExtendedAttributeManager.getDefaultCrdentialEncoder()
.
inside the function we will just add : return getCredentialEncoder();
I didn't do that because i will need this function only in the UP
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that getDefaultCredential is clearer, and safer.
WDYT ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK i will do it.
Done |
Your PR triggers too many exo-ci builds! Please finish your work and then, set your PR ready! Thank you |
baf47f4
to
f581d94
Compare
…hm - EXO-65151 - Meeds-io/MIPs#69 Prior to this change, As a part of the upgrade mechanism of old hashed passwords we need to create an UP to re-hash old passwords by the new hash algo. This PR creates an UP which has to update the old passwords hash by the new algo by re-hashing and adding the needed salt attribute.
Kudos, SonarCloud Quality Gate passed! |
…hm - EXO-65151 - Meeds-io/MIPs#69 (#161) Prior to this change, As a part of the upgrade mechanism of old hashed passwords we need to create an UP to re-hash old passwords by the new hash algo. This PR creates an UP which has to update the old passwords hash by the new algo by re-hashing and adding the needed salt attribute.
Prior to this change, As a part of the upgrade mechanism of old hashed passwords we need to create an UP to re-hash old passwords by the new hash algo. This PR creates an UP which has to update the old passwords hash by the new algo by re-hashing and adding the needed salt attribute.